chore: transfer firestore definition from root to prod workspace and create dev database instance#7
chore: transfer firestore definition from root to prod workspace and create dev database instance#7ASPactores wants to merge 4 commits into
Conversation
…rations for production firebase instance
There was a problem hiding this comment.
Pull request overview
Moves Firestore provisioning responsibility out of the root workspace and into the prod workspace, while introducing a separate “dev” Firestore database instance configuration.
Changes:
- Removed the Firestore module and output from
workspaces/root. - Added a
dev_gcp_firestoremodule inworkspaces/prodplus new prod workspace variables/tfvars for database management. - Updated the shared
modules/gcp/firestoremodule to support optionally skipping database creation (create_database) viacount, and adjusted outputs accordingly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| workspaces/root/main.tf | Removes root workspace Firestore module wiring. |
| workspaces/root/outputs.tf | Removes root workspace Firestore output. |
| workspaces/prod/main.tf | Adds dev_gcp_firestore module and comments out prod Firestore module block. |
| workspaces/prod/outputs.tf | Outputs Firestore DB id (currently from dev module) and includes commented prod output. |
| workspaces/prod/variables.tf | Adds Firestore-related workspace inputs (create_database, firestore_delete_protection_state). |
| workspaces/prod/terraform.tfvars | Sets Firestore-related tfvars values for prod workspace. |
| modules/gcp/firestore/main.tf | Adds count gate for optional database creation. |
| modules/gcp/firestore/variables.tf | Adds create_database variable to control database creation. |
| modules/gcp/firestore/outputs.tf | Updates outputs to handle count = 0 resource case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Production Firestore instance outputs (uncomment when needed). | ||
| #output "gcp_firestore_db_id" { | ||
| # description = "ID of the Firestore database in prod." | ||
| # value = module.gcp_firestore.firestore_db_id | ||
| #} | ||
|
|
||
| output "gcp_firestore_db_id" { | ||
| description = "ID of the Firestore database in prod." |
There was a problem hiding this comment.
The commented "Production Firestore instance outputs" block uses the same output name (gcp_firestore_db_id) as the active output below. If someone follows the instruction to "uncomment when needed" without also removing/renaming the existing output, Terraform will fail with a duplicate output name error. Suggest adjusting the comment to call out the required rename/removal, or define distinct output names for prod vs dev.
There was a problem hiding this comment.
Apply suggestion
There was a problem hiding this comment.
Applied in a48ce3c — renamed the output to dev_gcp_firestore_db_id with description "ID of the Firestore database in dev.", and updated the commented-out prod output block name to gcp_prod_firestore_db_id so uncommenting it won't cause a duplicate name conflict.
| module "dev_gcp_firestore" { | ||
| source = "../../modules/gcp/firestore" | ||
|
|
||
| project_id = var.gcp_project_id | ||
| region = var.gcp_region | ||
| project_id = var.gcp_project_id | ||
| region = var.gcp_region | ||
| database_name = "dev-payloadcms-db" | ||
| create_database = var.create_database | ||
| delete_protection_state = "DELETE_PROTECTION_DISABLED" | ||
|
|
There was a problem hiding this comment.
create_database is documented as controlling whether the prod workspace creates/manages the Firestore database, but it is also wired into the dev_gcp_firestore module. With the current wiring, turning create_database = false would skip creating the dev database as well. Consider splitting this into separate flags (e.g., dev vs prod) or updating the variable name/description so it accurately reflects that it gates both databases.
| description = "Whether the prod workspace should create/manage the Firestore database." | ||
| type = bool | ||
| default = true | ||
| } | ||
|
|
||
| variable "firestore_delete_protection_state" { | ||
| description = "Delete protection state for the prod Firestore database." | ||
| type = string | ||
| } | ||
|
|
There was a problem hiding this comment.
The create_database / firestore_delete_protection_state variables are now part of the prod workspace interface, but firestore_delete_protection_state is only referenced in commented-out code and create_database currently affects the dev DB module too. Consider aligning variable names/descriptions with actual behavior (or wiring the prod module via a conditional instead of commented code) to avoid confusing/unused workspace inputs.
| description = "Whether the prod workspace should create/manage the Firestore database." | |
| type = bool | |
| default = true | |
| } | |
| variable "firestore_delete_protection_state" { | |
| description = "Delete protection state for the prod Firestore database." | |
| type = string | |
| } | |
| description = "Whether this workspace should create/manage the Firestore database resources it currently controls." | |
| type = bool | |
| default = true | |
| } |
| output "firestore_db_id" { | ||
| description = "The Firestore database resource name." | ||
| value = google_firestore_database.this.name | ||
| value = try(google_firestore_database.this[0].name, var.database_name) | ||
| } |
There was a problem hiding this comment.
When create_database = false, firestore_db_id falls back to var.database_name. That changes the output semantics from "resource attribute" to "input echo" and can be misleading (especially given the description says "resource name"). Consider returning null (or a clearly documented placeholder) when the resource isn't created, or updating the output description/name to reflect that it may simply return the configured database ID when unmanaged.
| output "gcp_firestore_db_id" { | ||
| description = "ID of the Firestore database in prod." |
There was a problem hiding this comment.
gcp_firestore_db_id is described as the Firestore database in prod, but its value is coming from module.dev_gcp_firestore. This will mislead consumers (and can cause accidental use of the dev DB where prod is expected). Consider either renaming the output to something like dev_gcp_firestore_db_id / updating the description, or reintroducing a separate prod output when the prod module is enabled.
| output "gcp_firestore_db_id" { | |
| description = "ID of the Firestore database in prod." | |
| output "dev_gcp_firestore_db_id" { | |
| description = "ID of the Firestore database in dev." |
… fix commented prod output name Agent-Logs-Url: https://github.com/DurianPy-Davao-Python-User-Group/durianpy-root-infra/sessions/ef7be7b8-f54e-48bd-bb05-a846bc9910f3 Co-authored-by: ASPactores <91829714+ASPactores@users.noreply.github.com>
No description provided.